Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Theme Templates: Fix display issues of Offline & Day of Event templates across themes #316

Merged
merged 5 commits into from
Jan 22, 2020

Conversation

ryelle
Copy link
Contributor

@ryelle ryelle commented Jan 10, 2020

The templates on some default themes use different markup, which causes display issues when trying to use the same templates across themes. To fix this, we should stop trying to fully emulate all themes. This PR removes the global Day of Event template, since the Day of Event stub is created and can use a regular page template. The offline page is called differently, but instead of trying to match the theme directly, we can create a very simple page, which inherits the basic style of the active theme.

Fixes #205, fixes #206

Twenty Thirteen
twentythirteen

Twenty Fifteen
twentyfifteen

Twenty Seventeen
twentyseventeen

Twenty Twenty
twentytwenty

How to test the changes in this Pull Request:

Day of Event:

  1. Before switching to this branch, set a page to have the Day of Event template
  2. Switch to the branch
  3. Check that the page from step 1 still displays correctly

Offline:

  1. Make sure the PWA plugin is active
  2. View the offline page: yoursite.wordcamp.test/?wp_error_template=offline
  3. Try with multiple themes, it should not look broken (just simple, see screenshots for example)

@ryelle ryelle added [Status] Needs Review [Component] Theme Templates Theme-agnostic templates like Day of Event, Offline [Component] PWA Service workers, manifest labels Jan 10, 2020
@ryelle ryelle self-assigned this Jan 10, 2020
@ryelle ryelle added this to the PWA Initial Version milestone Jan 10, 2020
Copy link
Contributor

@coreymckrill coreymckrill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested locally:

  • Twenty Nineteen's layout looks broken, probably what you're describing as "unexpectedly full width"
  • Twenty Fourteen doesn't have the full width problem per se, but it has some margin issues:

twentyfourteen

ryelle and others added 5 commits January 17, 2020 09:58
Twenty Twenty uses a much different markup format for pages, so it's easier to match by using an entirely separate template. This approach should be considered a "patch", if the PWA merges into core, default themes will provide these templates.
@ryelle ryelle force-pushed the update/global-templates-theme-support branch from a823732 to 3755ab3 Compare January 17, 2020 18:53
@ryelle
Copy link
Contributor Author

ryelle commented Jan 17, 2020

Okay, I've removed the Day of Event template, and tweaked the offline template so that it's very, very simple. I'll update the description to reflect the actual PR now.

Copy link
Contributor

@coreymckrill coreymckrill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Day of Event: 👍 while testing locally

Offline: It looks good while testing locally on everything except Twenty Twelve (which, maybe we don't care about so much at this point?)

twenty-twelve

@ryelle
Copy link
Contributor Author

ryelle commented Jan 22, 2020

It looks good while testing locally on everything except Twenty Twelve

I think that's fine, only 3 sites in my "2019+" dataset use Twenty Twelve… and it's not totally unreadable, just not great.

@ryelle ryelle dismissed coreymckrill’s stale review January 22, 2020 15:43

latest iteration is approved

@ryelle ryelle merged commit b78cf51 into production Jan 22, 2020
@ryelle ryelle deleted the update/global-templates-theme-support branch January 22, 2020 15:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Component] PWA Service workers, manifest [Component] Theme Templates Theme-agnostic templates like Day of Event, Offline
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Day of Event Template: Test with all themes Offline Template: Test with all themes
2 participants